Build/Test Tools: Teach PHPStan WordPress hash notation - #13233
Build/Test Tools: Teach PHPStan WordPress hash notation#13233swissspidy wants to merge 22 commits into
Conversation
Core documents the contents of an array argument with a nested list of `@type` tags. PHPStan reads that hash as free text, so the value stays a plain `array` and nothing inside it is typed, and a shape that should be visible to the analysis has to be written a second time as a `@phpstan-param` or `@phpstan-return` beside the hash that already describes it. `HashNotationVisitor` translates the hash into the array shapes PHPStan understands, so the documentation core already writes serves the reader and the analysis alike. Across `src/wp-admin`, `src/wp-includes` and the bundled themes it derives 394 tags from the 465 hashes it can see. A hash whose translation would be a guess is left alone, so the visitor only ever narrows a type and never contradicts one. A `@phpstan-param` or `@phpstan-return` written by hand always wins; the declared type has to name a bare `array`; the hash has to be well formed; and a parameter taken by reference is skipped, since PHPStan checks those in both directions and a shape there would constrain every caller's variable rather than describe what the function reads. Keys of a `@param` hash are optional and its shape is left open, because the hash lists the keys core reads rather than the only keys a caller may pass. Keys of a `@return` hash are required and its shape is sealed, since they describe a value core itself builds, unless the description marks one `Optional.` Two kinds of hash are left for later. A `@var` hash on a property is inherited by every subclass and has to accept its own default, so a shape there would say more than the hash does. An `object` hash, such as the one on `get_taxonomy_labels()`, would need the docblock to name the class rather than `object`, because PHPStan's object shapes are structural and one derived for a `stdClass` is no longer assignable to a property declared `stdClass`. The translation follows the one php-stubs/wordpress-stubs performs when generating stubs, which is how the WordPress flavor of PHPDoc reaches PHPStan today for plugins and themes. Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com> Co-Authored-By: Claude <noreply@anthropic.com>
With hash notation translated into array shapes, the analysis checks these hashes against the code for the first time, and reports where the two disagree: - `WP_Http::processHeaders()` documents a `newheaders` key. The array it returns has `headers`. - `wp_edit_attachments_query()` documents `post_mime_types` and `avail_post_mime_types` keys. It returns the two values positionally, so they are `$0` and `$1`, as `wpdb::parse_db_host()` already writes them. - `WP_List_Table::get_views_links()` documents `url`, `label` and `current` as keys of `$link_data`. They are keys of each link in it, which its own `@return` line says: "Keys match the `$link_data` input array." Every caller passes a keyed array of links. - `wp_check_php_version()` sets `is_lower_than_future_minimum` on every array it returns, and both callers read it, but it is not documented. - `wpdb::parse_db_host()` documents the port as `string|null`, right below the line of code that casts it with `absint()` and the comment saying "Port cannot be a string; must be null or an integer." - `wp_xmlrpc_server::wp_editPage()` documents its content argument as a string. It is the content struct, which the method writes `post_type` into before passing it on. - `WP_Http::request()` documents `headers` as a `CaseInsensitiveDictionary`. A non-blocking request returns an empty array for it. - `wp_upload_bits()` documents `file`, `url` and `type` alongside `error`. Only `error` is set when the upload fails. Two returns cannot be described by a hash at all, because they are one shape or another rather than one shape with optional keys, so they gain a `@phpstan-return` beside the hash, as `wp_upload_dir()` and `_wp_handle_upload()` already have: `wp_font_dir()`, which returns what `wp_upload_dir()` returns, and `get_avatar_data()`, whose returned array also carries every argument passed to it, as its own description says. What remains are call sites passing an argument the documented shape does not accept, which is recorded in the baselines rather than resolved here. Each is a hash and a caller disagreeing about a key, and worth its own look. Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com> Co-Authored-By: Claude <noreply@anthropic.com>
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
An object shape in PHPStan is structural, so one derived from a bare
`@return object { ... }` describes the members and nothing else. That made
it useless where core actually puts those values: `WP_Taxonomy::$labels`
and `WP_Post_Type::$cap` are declared `stdClass`, which a bare
`object{...}` is not, so assigning one was an error and the hashes had to
be skipped.
Naming the class in the docblock resolves it. A hash on a class produces
an intersection, `stdClass&object{...}`, which is still the class and now
also carries the members, so it is assignable to a property declared
`stdClass` and reads of those members are typed. The three returns that
build one with a cast say `stdClass` rather than `object` to match what
they return: `get_taxonomy_labels()`, `get_post_type_capabilities()` and
`wp_get_scheduled_event()`.
An intersection inside a union is parenthesized, so
`wp_get_scheduled_event()` reads `(stdClass&object{...})|false`.
Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com>
Co-Authored-By: Claude <noreply@anthropic.com>
The docblock the visitor builds is longer than the one in the file, so the original start line and file position no longer describe where its text lives. `GlobalDocBlockVisitor` leaves both off for the same reason; this one was passing them through. No change to what is derived locally, and the analysis stays green. Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com> Co-Authored-By: Claude <noreply@anthropic.com>
The cache CI keeps for PHPStan holds more than the analysis results. PHPStan also stores what it read out of each source file there, the docblocks and signatures it found, keyed by that file's contents and nothing else. A parser node visitor changes what reading a file yields without changing the file, so a cache written before `HashNotationVisitor` existed answers with the docblocks core had before it, and no shape is derived from any hash. That is what the run on this branch was reporting. It restored the cache trunk's run on the base commit wrote, so every file this branch does not touch came back from that cache without a shape, the baselines written against those shapes matched nothing, and PHPStan reported them under `ignore.unmatched` and `ignore.count`. Reproduced by analysing the base commit with an empty `.cache` and then analysing this branch on top of the cache that left behind: the same reports, on the same lines, down to `register_setting` printing trunk's `expects array, string given`. With `.cache` cleared the branch is green, which is why it looked green locally. Keying the cache on `phpstan.neon.dist` and the sources in `tests/phpstan` keeps a run from restoring a cache that predates either. The baselines are left out of the key: they only decide which reported errors are ignored, PHPStan invalidates the results cache on a configuration change by itself, and including them would discard the whole cache every time one is regenerated. Nothing keys the cache for a local run, so `tests/phpstan/README.md` says to clear it by hand after changing anything in that directory. Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com> Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Are1pyfSoWBPabAmc4vPP1
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @claude. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Follow-up to review feedback on the pull request. `wp_upload_dir()` never returns the union its hand-written `@phpstan-return` describes. `_wp_upload_dir()` always sets `error` to `false`, and when the directory cannot be created `wp_upload_dir()` overwrites only that key, so `path`, `url`, `subdir`, `basedir` and `baseurl` are there either way. It is one shape whose `error` is `non-empty-string|false`, and whose `subdir` is empty when the year/month option is off, so a `string` rather than a `non-empty-string`. `wp_get_upload_dir()` carried the same tag with the same two mistakes. With that corrected, `wp_font_dir()` needs no tag of its own: it returns `wp_upload_dir()`'s value unchanged, and the shape its own hash describes is exactly what `wp_upload_dir()` now promises. The tag added for it here is gone again. `wp_upload_bits()` does return two shapes, so it gains the `@phpstan-return` its hash cannot express: the file it wrote, with `error` set to `false`, or an array carrying the message. That arm is left open with `...`, because two of the failures return `wp_upload_dir()`'s array with the message written into it, and its other keys come along; the contract is that `error` is there. Its `type` is `wp_check_filetype()`'s, which is `false` for a name matching no mime type, so the hash says `string|false`. `WP_Http::processHeaders()` returns the headers keyed by lowercased name, each value the header, or every value of a header sent more than once. That is `array<string, string|string[]>`, which the hash now says instead of `array`. `get_post_mime_types()` returns groups keyed by mime type, each a three-item array of the group's plural name, the label for its "Manage" screen, and the count strings `_n_noop()` builds. Typing it, and the `post_mime_types` filter it returns through, lets `wp_edit_attachments_query()` describe the first of the two values it returns, rather than calling it `array[]`, which said the wrong thing about its keys as well. `wp_editPage()` hands its content struct to `mw_editPost()` and reads none of it, so its `@type` points at `mw_newPost()`, where core documents the keys the struct takes, rather than repeating them. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018CG1pXsNoUnbjzti49c1Zy
A hash the visitor skips is skipped silently: no tag is derived, nothing
reports it, and the value keeps the type it had. Review found several, and one
hash being translated that should not be.
`has_phpstan_counterpart()` searched the whole docblock for a hand-written
`@phpstan-param` and the variable, any distance apart. A `@phpstan-param`
written for one parameter therefore covered the next, and a conditional
`@phpstan-return` naming a parameter covered it too. `wp_die()` has both, so its
eight-key `$args` hash was never read. The search now walks the tags
`split_tags()` has already produced and stays inside the one that was written.
Across every function docblock in `src`, that is the only tag it changes.
Types are checked by PHPStan's own type parser rather than by a description of
the characters a type may contain. That description had already drifted from the
grammar it stands in for: it excluded `(` and `)`, which left the parentheses
`split_union()` counts unreachable and dropped `Translation_Entry::__construct()`
over a single `@type (string|null)[]`. It would have done the same to `?string`
or a `callable(): void`.
`Array` fell through the `array`/`object` test, which was case-sensitive while
every other type name was matched case-insensitively, and came out as an
intersection with a class that does not exist. Both comparisons are normalized
now, and the keyword list is a constant.
`Optional.` is read as the marker the documentation standard defines: the word
opening the description, with its period. It was matching the word anywhere in a
description built by joining continuation lines, which took it out of prose such
as "receives optional mixed input". It now also overrides the numbered-key rule,
so the twenty XML-RPC methods documenting `@type int $4 Optional. Publish flag.`
describe an argument a caller may leave off, while prose that merely opens with
the word, as the "Optional self closing slash" of a match array whose every group
is always set, marks nothing.
Three smaller reads in `parse_entries()`: a `@type` separated by a tab is
recognized, matching what `split_tags()` already accepted; a line below a nested
hash no longer joins the description of the entry that opened it, whose own
description was read before its `{`; and the intro line of a nested hash becomes
that entry's description, which is where an `Optional.` for a whole block is
written.
A `...$N` entry beside named keys is how core writes "and the rest". It was
dropping the hash, and now leaves the shape open, which says the same thing:
`wp_maybe_grant_site_health_caps()` and the three query classes derive again. An
object hash that would have to stay open is skipped instead, because PHPStan's
object shapes have no `...`: sealing the theme data
`WP_Theme_Install_List_Table::single_row()` documents would have reported every
member the wordpress.org API adds and the hash does not name.
A union of a bare `array` and a class is no longer called ambiguous. A `WP_Error`
carries no keys, so in `array|WP_Error`, core's most common return signature, the
hash is about the array. Thirteen hashes derive because of it, `WP_Http::request()`
among them.
The replacement splicing the derived tags into the docblock is built by a
callback, so a `$0` in a derived tag cannot be read as a backreference.
`tests/phpstan/README.md` follows the rules as they now read, and gains the one
other way a stale cache is reached: analysing a subset of the tree stores, for
every file merely read on its behalf, reflection with no shape in it.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CG1pXsNoUnbjzti49c1Zy
The key added on this branch kept the old cache name and prefixed it with a hash of the analysis configuration, so every key it writes still begins with `phpstan-result-cache-` — which is what the workflow this branch replaces restores on. A branch that has not merged this commit runs the workflow it has, prefix-matches the caches written here, and analyses without the extensions against reflection derived with them. That is the failure this branch exists to close, reaching it from the other side. Renaming the cache leaves those runs restoring only their own. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018CG1pXsNoUnbjzti49c1Zy
…port. `wp_die()` documents `@type int $response`, but `wp_send_json()` passes null and four of the handlers test for it: "This is intentional. For backward-compatibility, support passing null here." The type is `int|null`. `WP_Date_Query::__construct()` documented `column`, `compare` and `relation` inside its list of clauses, one level below where the constructor reads them. `WP_Meta_Query` and `WP_Tax_Query` document the same keys at the top level, which is where all three read them. `get_taxonomy_labels()` sets `menu_name` on the defaults it builds and did not document it. That matters now the hash is the shape: a member it leaves out is a member reading is reported for. `wp_upload_bits()` returns the message a `wp_upload_bits` filter handed it, which may be an empty string, so the error arm of its shape is `string` rather than `non-empty-string`. Two calls the hashes report are wrong rather than mistyped, so they are fixed rather than baselined: - `WP_MS_Themes_List_Table::column_description()` passes `additional_classes` as a string. `wp_get_admin_notice()` reads that key only when it is an array, so the broken theme notice has never had the `inline` class it asks for. The sibling call in the same file passes an array. - Twenty Nineteen passes `title_reply` as null. `comment_form_title()` echoes it either way, so an empty string renders the same and is the documented type. The one baseline whose message the new shapes change is regenerated with it. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018CG1pXsNoUnbjzti49c1Zy
Each of these was recorded in a baseline when the hashes became shapes rather than fixed, and each is a `@type` that says less than the function accepts. In three of them the description sitting beside it already said so. `wp_nav_menu()` documents `$container` as a string, then tests `is_string( $args->container )` before using it — a guard with nothing to guard against unless the value can be something else, which is what Twenty Twenty One's footer passes. `wp_list_pages()` documents `$title_li` as a string in a sentence reading "Passing a null or empty value will result in no heading". Twenty Twenty passes false. The type and the sentence now agree on falsy. `WP_Ajax_Response::add()` documents `$position` as a string in a sentence reading "Accepts 1 (bottom), -1 (top)". Its `$id` is documented `int|WP_Error`, while every caller in wp-admin passes a `$_POST` value or `$comment->comment_ID`, both strings. `get_bookmarks()` documents `$category` as a comma-separated list of IDs, then assigns `$parsed_args['category'] = $term->term_id` into it itself when `$category_name` was given instead. Two calls are corrected rather than their documentation, because the value the documentation names already existed. Twenty Twenty passes `''` for `wp_nav_menu()`'s `$fallback_cb`, where the docblock and Twenty Twenty One's own footer write `false`; both are falsy, and `wp_nav_menu()` tests truthiness before `is_callable()`. `WP_Customize_Manager` passes `0` for `get_pages()`'s documented `bool $hierarchical`. Eleven baseline entries go with them, taking what this branch adds to `argument.type` from twenty-two to nine. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018CG1pXsNoUnbjzti49c1Zy
An earlier commit on this branch stopped deriving a shape for any object hash
that had to stay open, on the reasoning that PHPStan's object shapes carry a
fixed member list and no `...`, so sealing one would report every member the
hash does not name. That is true of a bare `object{ ... }`. It is not true of
the intersection the visitor actually emits for a hash on a named class.
`stdClass&object{ ... }` is not sealed: the intersection leaves the class to
say what else may be read, and a `stdClass` accepts anything. Reading an
undocumented property off one is reported for the bare shape and allowed for
the intersection, which is the case that matters here, since core builds these
values with `wp_parse_args()` and the hash names the keys it reads rather than
the only keys present.
So only a bare `object` hash that has to stay open is skipped now.
`WP_Theme_Install_List_Table::single_row()` derives its shape again, and it is
the only docblock in `src` the distinction moves. The rules in the class
docblock and in `tests/phpstan/README.md` said the wrong thing and now say this
one.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CG1pXsNoUnbjzti49c1Zy
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Review asked for the shape on _wp_upload_dir() as well, rather than only on the two functions that return its value filtered. It is the same shape wp_upload_dir() documents, except that error is always false: the error string is only ever attached by wp_upload_dir() after trying to create the directory, and the upload_dir filter that could add one does not run here. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQBXKmmaicaU2z8HhPnxs2
Trunk widened WP_Customize_Setting's default from string to mixed, so the shape derived from its constructor hash now accepts the array default WP_Customize_Nav_Menus passes, and the entry written against the narrower shape matched nothing. Regenerated with composer phpstan:baselines, which removes only that entry. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQBXKmmaicaU2z8HhPnxs2
Trunk documented register_setting()'s sanitize_callback as nullable, so the shape derived from its hash now reads (callable(): mixed)|null and the entry written against the older wording matched nothing. Regenerated with composer phpstan:baselines, which rewrites only that entry. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQBXKmmaicaU2z8HhPnxs2
Follows up on the suggestion in #13220: rather than writing PHPStan types beside the hashes that already describe the same shapes, teach PHPStan to read the hashes.
tests/phpstan/HashNotationVisitor.phpis a parser node visitor, in the same shape as theGlobalDocBlockVisitoralready in that directory. It reads hash notation from@paramand@returntags and appends the equivalent shape:becomes, to PHPStan:
Nothing is written to disk; the docblock is rewritten in the AST, so the source keeps only the hash. Across
src/wp-admin,src/wp-includesand the bundled themes it derives 383 tags in 137 files, from the 465 hashes it can see that do not already carry a hand-written one.The translation is the one php-stubs/wordpress-stubs performs when generating stubs, which is how the WordPress flavor of PHPDoc reaches PHPStan today for plugins and themes. Doing it here means core's own analysis gets the same types, and that a shape has one source rather than two that can drift apart.
Objects
A hash on an object works too, including the one
get_taxonomy_labels()carries that #13220 documents. The shape is intersected with the class rather than derived bare:PHPStan's object shapes are structural, so a bare
object{…}is not astdClassand could not be assigned toWP_Taxonomy::$labelsorWP_Post_Type::$cap, which are declared as one. Intersecting keeps both: the value stays the class it is documented as, and its members are typed. The three returns that build one with a cast now saystdClassrather thanobject, matching what they return.What it does not translate
A hash whose translation would be a guess is left alone, so the visitor only ever narrows a type and never contradicts one:
@phpstan-paramor@phpstan-returnalways wins. Hash notation cannot express everything a type can — a function returning one shape or another, for instance — so a shape tuned in the source is never overwritten. 35 hashes are covered that way today.arrayorobject, or a class, alone or as one member of a union likestring|array. A type already more specific than the hash, such asarray<string, string|bool>, is left as written.{closed by a}on its own line, every@typecarrying a type and a$name.@varhashes on properties are skipped. A property declaration is inherited by every subclass and has to accept its own default, so a shape there would say more than the hash does.Keys of a
@paramhash are optional at every level, and the shape is left open with a trailing..., because a hash lists the keys core reads rather than the only keys a caller may pass. Keys of a@returnhash are required and its shape is sealed, since they describe a value core builds — unless the description marks oneOptional., which the visitor honors, asWP_Http::request()already writes for$filename.Hashes on hook docblocks — around 170 of them — are not attached to a function, so they are outside what a node visitor sees. The value a filter passes stays typed by the existing hook extensions.
The cache has to know about the extensions
The last commit is not about hash notation, but this pull request is what turned it up.
.cacheholds more than the analysis results. PHPStan also stores what it read out of each source file there — the docblocks and signatures it found — keyed by that file's contents and nothing else. A parser node visitor changes what reading a file yields without changing the file, so a cache written beforeHashNotationVisitorexisted answers with the docblocks core had before it, and no shape is derived from any hash. Nothing fails or warns; the analysis simply runs against types that are no longer derived.That is what the earlier runs on this branch were reporting. CI restored the cache trunk's run had written, so every file this branch does not touch came back without a shape, the baselines written against those shapes matched nothing, and PHPStan reported them under
ignore.unmatchedandignore.count— down toregister_setting()printing trunk'sexpects array, string given. Reproduced by analysing the base commit with an empty.cache, then analysing this branch on top of what that left behind.The results cache alone is not the problem: PHPStan invalidates that itself on a configuration change, and says so under
-vv. What survives is the per-file reflection, which it has no way to know is stale. So the workflow now keys its cache onphpstan.neon.distand the sources intests/phpstan, andtests/phpstan/README.mdsays to clear.cacheby hand after editing anything in that directory, since nothing keys it for a local run.What it found
Turning it on made the analysis check these hashes against the code for the first time. The second commit fixes what it reported:
WP_Http::processHeaders()newheaderskey; the array it returns hasheaderswp_edit_attachments_query()post_mime_typesandavail_post_mime_typeskeys; it returns the two values positionallyWP_List_Table::get_views_links()url,label,currentas keys of$link_data; they are keys of each link in it, which its own@returnline sayswp_check_php_version()is_lower_than_future_minimum, and both callers read it, but it is not documentedwpdb::parse_db_host()string|null, right below theabsint()cast and the comment "Port cannot be a string; must be null or an integer"wp_xmlrpc_server::wp_editPage()post_typeintoWP_Http::request()headersas aCaseInsensitiveDictionary; a non-blocking request returns an empty arraywp_upload_bits()file,urlandtypealongsideerror; onlyerroris set when the upload failswp_font_dir()andget_avatar_data()return one shape or another rather than one shape with optional keys, which a hash cannot say, so each gains a@phpstan-returnbeside its hash the waywp_upload_dir()and_wp_handle_upload()already do.It also surfaced call sites passing an argument the documented shape does not accept. Most of those are a docblock and a caller disagreeing about a key, and are a change of their own, so they are split out into #13235 — which also removes three baseline entries on its own. With that landed, what stays baselined here is only the handful no docblock change reaches, where the array arriving at the call has no statically known keys.
Testing
composer phpstanis green, on CI and locally, from a cleared result cache.Trac ticket: https://core.trac.wordpress.org/ticket/65817
This may want its own ticket rather than sharing 65817, which is about the docblocks themselves — happy to move it.
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Used for: writing the visitor and this description, measuring its effect on the analysis, and drafting the documentation corrections it surfaced.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.